-
Notifications
You must be signed in to change notification settings - Fork 47k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing the instantiation of customized builtin elements (related to custom elements) #9313
Fixing the instantiation of customized builtin elements (related to custom elements) #9313
Conversation
@@ -552,7 +552,7 @@ ReactDOMComponent.Mixin = { | |||
div.innerHTML = `<${type}></${type}>`; | |||
el = div.removeChild(div.firstChild); | |||
} else if (props.is) { | |||
el = ownerDocument.createElement(type, props.is); | |||
el = ownerDocument.createElement(type, {is: props.is}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://dom.spec.whatwg.org/#dom-document-createelement.
The is
prop indicates that the react element should be a customized builtin element.
@@ -1045,7 +1045,7 @@ describe('ReactDOMComponent', () => { | |||
ReactDOM.render(<div is="custom-div" />, container); | |||
expect(document.createElement).toHaveBeenCalledWith( | |||
'div', | |||
'custom-div', | |||
{is: 'custom-div'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document.createElement
should not have a second argument of a string, but rather an options
object
Updated this to fix the build -- I had forgotten to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Fiber tests are failing (that’s a new engine that’s hidden behind a flag).
There is equivalent code in ReactDOMFiberComponent.js
, could you please fix it too and then run scripts/fiber/record-tests
?
@gaearon thanks for the help -- I have updated ReactDOMFiberComponent as suggested, but the fiber tests are failing for me locally. The same tests fail for me whether I'm on the master branch or on my branch, so I don't think it's actually related to my code changes. And it changes the tests-failing and tests-passing files, even on the master branch. So what I have done is push my change to ReactDOMFiberComponent without also adding my changes to The offending tests are:
Maybe related to react@15.5, since it's related to PropTypes??? I am not sure if it's just my local env that is causing issues or if maybe the tests-failing.txt file has not been updated since react@15.5?? Would appreciate any help you could offer. |
Pretty sure this is the case. MDN:
The feature itself was originally added in #6570, and I don’t think it was extensively tested or updated to match the latest spec. For safety I’ll merge this to master but won’t backport to 15.x. Thanks! |
The custom element spec, combined with the document.createElement spec explain that in order to create a DOM element as a customized built-in element, you should call document.createElement with an object as the second parameter. Customized builtin elements are a type of custom element that allow you to extend native DOM elements. However, React currently provides the second parameter to
document.createElement
as a string instead of an object. As far as I can tell, that is just a bug.I am not sure of the full history of the custom elements spec, and so it might be possible that the spec used to say the second argument to createElement should be a string. I'm also not sure if anybody is depending on the behavior being like it is right now, or if this change would be considered a breaking change or not for React. My thoughts are that it is a bug fix, but I may be lacking some context here.